Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alias-relate: add fast reject optimization #124852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 7, 2024

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels May 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lcnr
Copy link
Contributor Author

lcnr commented May 7, 2024

this unfortunately does not fix the hang in rayon. closing while we're trying other approaches

@lcnr lcnr closed this May 7, 2024
@lcnr lcnr reopened this May 12, 2024
Comment on lines 35 to 36
if self.fast_reject_unnameable_rigid_term(param_env, lhs, rhs)
&& self.fast_reject_unnameable_rigid_term(param_env, rhs, lhs)
Copy link
Contributor Author

@lcnr lcnr May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn 🤔 if only there was a reason why this fast path didn't fix the hang in rayon

this literally never triggers 🙃 it needs to be ||

@lcnr
Copy link
Contributor Author

lcnr commented May 12, 2024

if there's (): Trait<Assoc = TAIT> and TAIT is then defined to a placeholder, we can inject it into the env this way, even if it should error later. not yetsure whether and how we should handle this

@bors
Copy link
Contributor

bors commented May 20, 2024

☔ The latest upstream changes (presumably #125324) made this pull request unmergeable. Please resolve the merge conflicts.

return false;
}

let mut referenced_placeholders =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why IgnoreAliases::Yes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this fast reject could be quickly side-stepped w/ something like Mirror<!A> alias-relate Mirror<!B>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: It occurred to me last night while I was sleeping that we ignore aliases here b/c e.g. Alias<!A> could normalize to sth not mentioning !A.


let mut referenced_placeholders =
self.collect_placeholders_in_term(rigid_term, IgnoreAliases::Yes);
for clause in param_env.caller_bounds() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to explain in detail what this is searching for. Specifically, we disqualify any of our rigid term's placeholders if they show up in projection goals, since that may assist in normalizing.

e.g. if we have

rigid = W<!A>
alias = Alias<!B>
param-env = [Alias<!B> normalizes-to W<!A>, ...]

then we exclude !A from our rigid placeholder list, right?

I think this may also be worth mentioning that this doesn't really care about how the placeholders show up in the term of the projection predicate, since this is really just a quick heuristic.

}
}

if referenced_placeholders.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a somewhat redundant note, but maybe just say "empty set is a subset of everything, so no need to compute placeholders of alias"

}

let alias_placeholders = self.collect_placeholders_in_term(alias, IgnoreAliases::No);
// If the rigid term references a placeholder not mentioned by the alias,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If the rigid term references a placeholder not mentioned by the alias,
// If the rigid term references a placeholder not mentioned by the alias, and isn't potentially reachable by a projection-predicate,

or something, to explain the way that the projection predicate search above influences this search

@compiler-errors
Copy link
Member

My only outstanding question was answered by myself. I think the only tweaks I want are more thorough comments bc this code is subtle.

r=me afterwards

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2024
@bors
Copy link
Contributor

bors commented Jun 18, 2024

☔ The latest upstream changes (presumably #126614) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@lcnr
ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

@alex-semenyuk alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants